Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Infrastructure for testing inter project imports and exports #6840

Merged
merged 6 commits into from
May 26, 2023
Merged

Infrastructure for testing inter project imports and exports #6840

merged 6 commits into from
May 26, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented May 25, 2023 •
edited
Loading

Pull Request Description

Infrastructure for testing inter project imports with few succeeding tests.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • All code follows the
    Scala,
  • All code has been tested:
    • Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label May 25, 2023
@JaroslavTulach JaroslavTulach self-assigned this May 25, 2023
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented May 25, 2023 •
edited
Loading

Trying to test how inter project imports work I realized I don't know what approch to copy!? There is:

None of them (as far as I can tell) shows how to test inter project imports. All the imports and exports are happening inside of a single project. This PR is my attempt to show how a testing infrastructure for inter project exports and imports could look like.

Of course, as Pavel told me last week, the problem is EditionManager - it is quite hardcoded and not easy to be extended. To overcome that I decided to expand micro-distribution with Test namespace modules. As the ImportsTest is using micro-distribution (good, as debugging imports on real Standard.Base is too huge task to swallow), one can import these Test modules in.

Turns out that the most important trick is to put the Test libraries into micro-distribution! Then one can do any kind of testing with it. 51f6cde shows how to use this from inside of ImportExportTest.

Is this OKeyish testing infrastructure Hubert, Pavel?

@JaroslavTulach JaroslavTulach changed the title Infrastructure for testing inter project imports Infrastructure for testing inter project imports and exports May 25, 2023
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented May 25, 2023 •
edited
Loading

OK, 18b5c29 probably wasn't good idea. There are two failing tests:

  • org.enso.interpreter.test.semantic.ImportsTest
  • org.enso.interpreter.test.semantic.PolyglotTest

I need to change them to get just the original getMessage and not the human readable one. Done in 9fbba8b

Akirathan
Akirathan approved these changes May 25, 2023
View reviewed changes
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I can't wait to add more libraries and more tests. This will make the transition to the better import system much smoother.

@radeusgd
Copy link
Member

the problem is EditionManager - it is quite hardcoded and not easy to be extended.

The edition configuration can be extended by defining a custom edition for the test project.

I'm not sure if there are good examples how to do it in a test project and I imagine there could be some obstacles to working with editions in a test environment (although ideally there shouldn't be any obstacles), but it should be very well doable to define custom editions for testing purposes.

@enso-bot enso-bot bot mentioned this pull request May 26, 2023
Merged
2 tasks
@JaroslavTulach
Copy link
Member Author

but it should be very well doable to define custom editions for testing purposes.

I don't think there is any EnsoLanguage "option" to mock in custom logic. There is the LANGUAGE_HOME_OVERRIDE and EDITION_OVERRIDE but those are already used by the test.

Thanks for the edition link:

  • "curated Edition file will guarantee that all libraries are compatible which simplifies version resolution" - yup that's a solution to the incompatibilities problem - just like Linux distribution like Ubuntu 22.0 guarantees all packages on official update centers are compatible
  • "if current edition defines a library ... that was already defined in the parent edition, ... the current edition takes precedence." - relates to Know it better! "paradox" - but correct decision in this case - curators of editions shall not be clueless
  • "you can shadow a repository name" - why? It can only create confusion. It should rather be forbidden to override repository definition when it has no effect on parent edition behavior
  • "custom edition search paths are ... defined by the ENSO_EDITION_PATH" - e.g. one can have special edition for testing, but it cannot be "virtual", it has to be on disk somewhere - similar to using micro-distribution as proposed by this PR
  • if the project has prefer-local-libraries set to true and if any directory on the ENSO_LIBRARY_PATH contains Foo/Bar - I guess we could set the ENSO_LIBRARY_PATH env variable in the test or create an option for EnsoLanguage to allow an engine to be configured as such. Then the Test libraries could be next to the other libraries used by the ImportsTest.scala

It is always valuable to be able to see the original design ideas.

@JaroslavTulach
Copy link
Member Author

Reviewers, please vote. "Thumbs up" it is OK as it is. "Thumbs down" to investigate the following change:

if the project has prefer-local-libraries set to true and if any directory on the ENSO_LIBRARY_PATH contains Foo/Bar

Should I move the Test/Logical_Export project away from micro-distribution and find a way to set ENSO_LIBRARY_PATH to point to engine/runtime/src/test/resources/ so all the sibling projects can be loaded by the ImportsTest.scala & other properly configured tests?

@Akirathan
Copy link
Member

Should I move the Test/Logical_Export project away from micro-distribution and find a way to set ENSO_LIBRARY_PATH to point to engine/runtime/src/test/resources/ so all the sibling projects can be loaded by the ImportsTest.scala & other properly configured tests?

I think that the current structure is sufficient. It allows us to (easily) create other libraries just by adding them to test/micro-distribution/lib/Test. There is no need to try to hack EditionManager just yet.

@JaroslavTulach JaroslavTulach merged commit e7ee2ca into develop May 26, 2023
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/TestingInterProjectImports branch May 26, 2023 15:33
@enso-bot
Copy link

enso-bot bot commented May 28, 2023

Jaroslav Tulach reports a new STANDUP for the last Friday (2023-05-26):

Progress: - import/export test infrastructure merged: #6840

Next Day: Bookclubs, bugfixing, ascribed types

Procrat added a commit that referenced this pull request May 30, 2023
@Procrat
Merge branch 'develop' into wip/procrat/fix-project-manager-unavailab…
4486d00 
…le-6756-6804

* develop: (22 commits)
  Coalesce graph editor view invalidations (#6786)
  Append warnings extracted before tail call execution (#6849)
  Detect and override hooks of the same kind (#6842)
  Dynamic app resampling and better performance measurements. (#6595)
  Show spinner when opening/creating a project, take #2 (#6827)
  Infrastructure for testing inter project imports and exports (#6840)
  Only initialise visualisation chooser if it is used. (#6758)
  Allow casting a Mixed column into a concrete type (#6777)
  Stop graph editing when in full-screen visualization mode (#6844)
  Handle `show-dashboard` event (#6837)
  Fix some dashboard issues (#6668)
  Fix JWT leak (#6815)
  Fix "set username" screen (#6824)
  Fallback to opened date when ordering projects (#6814)
  Various test improvements to increase coverage and speed things up (#6820)
  do not activate nested dropdowns together (#6830)
  Clearly select single specialization with enum dispatch pattern (#6819)
  Prevent incorrect application of list widget on incompatible expressions (#6771)
  Update GraalVM to 22.3.1 JDK17 (#6750)
  Import/export syntax error have more specific messages (#6808)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hubertp hubertp hubertp approved these changes

@jdunkerley jdunkerley jdunkerley approved these changes

@Akirathan Akirathan Akirathan approved these changes

@GregoryTravis GregoryTravis GregoryTravis approved these changes

@radeusgd radeusgd Awaiting requested review from radeusgd radeusgd is a code owner

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

Assignees

@JaroslavTulach JaroslavTulach

Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JaroslavTulach @radeusgd @Akirathan @hubertp @jdunkerley @GregoryTravis